Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Drop HSTS Preloading #1110

Merged
merged 4 commits into from
Aug 5, 2020
Merged

Drop HSTS Preloading #1110

merged 4 commits into from
Aug 5, 2020

Conversation

florimondmanca
Copy link
Member

@florimondmanca florimondmanca commented Aug 1, 2020

Maybe? See the rationale in #1102

Fixes #1102, closes #896

Essentially:

>>> import httpx
>>> r = httpx.get('http://www.paypal.com')  # -> Now kept over HTTP rather than forced to HTTPS.

I suppose if we want to move forward with this we'd want it in 0.14, rather than 1.0, since it might be a small breaking change?

Since this was an always-on feature not controlled by any options, I can't think of a smooth deprecation path, but any ideas welcome!

@florimondmanca florimondmanca added the api change PRs that contain breaking public API changes label Aug 1, 2020
@florimondmanca florimondmanca added this to the v0.14 milestone Aug 1, 2020
@tomchristie tomchristie mentioned this pull request Aug 2, 2020
@StephenBrown2
Copy link
Contributor

Perhaps hstspreloading could be kept, but not enforced by default, and a TRACE or DEBUG message emitted when a host is found on the list and doesn't match the preferred scheme?

@tomchristie
Copy link
Member

So, it's a wooly issue, but I think yes let's get this in and treat HSTS as appropriate as a browser feature, but not necessarily desirable in client library.

@tomchristie tomchristie merged commit 78cf16a into master Aug 5, 2020
@tomchristie tomchristie deleted the drop-hsts branch August 5, 2020 12:05
@StephenBrown2
Copy link
Contributor

I would hate for the great work in the hstspreload module to go unused. Would it be good to include a brief mention in the docs for those that do want that functionality?

@florimondmanca
Copy link
Member Author

florimondmanca commented Aug 5, 2020

I'm trying to think of how people could enable this without us having to support it in core with a flag…

I don't think a custom transport would be the way to go, since transports don't deal with client logic like "optionally modify the request URL".

Probably more like a client subclass, then…?

import httpx
import hstspreload

class HSTSPreloadMixin:
    def build_request(self, *args, **kwargs):
        request = super().build_request(*args, **kwargs)

        url = request.url

        if (
            url.scheme == "http"
            and hstspreload.in_hsts_preload(url.host)
            and len(url.host.split(".")) > 1
        ):
            port = None if url.port == 80 else url.port
            request.url = url.copy_with(scheme="https", port=port)

        return request

class AsyncClient(HSTSPreloadMixin, httpx.AsyncClient):
    pass

class Client(HSTSPreloadMixin, httpx.Client):
    pass

Incidentally I think this could also fit in a "middleware" kind of concept (#345, also mentioned in #984), but that's definitely not something we'll have 1.0.

@StephenBrown2
Copy link
Contributor

StephenBrown2 commented Aug 5, 2020

Possibly, but I was thinking of and even simpler approach/demonstration:

util.py

from typing import Union

from httpx import URL
from hstspreload import in_hsts_preload

def check_hsts(url: Union[str, URL]):
    if isinstance(url, str):
        url = URL(url)
    if in_hsts_preload(url.host):
        return url.copy_with(scheme="https")
    return url

...
main.py

import httpx

from util import check_hsts

httpx.get(check_hsts(the_url))

or with base_url:

with httpx.Client(base_url=check_hsts(the_url)) as client:
    client.get(path)

This was referenced Aug 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api change PRs that contain breaking public API changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider dropping HSTS preloading Document HSTS is still honored if allow_redirects=False
3 participants